Skip to content

test(browser): four checks no longer race a slow run - #1001

Merged
devswha merged 1 commit into
mainfrom
test/deflake-browser-checks
Oct 14, 2026
Merged

devswha merged 1 commit into
mainfrom
test/deflake-browser-checks

Conversation

@devswha

@devswha devswha commented Oct 13, 2026

Copy link
Copy Markdown
Owner

Change

Over the last day about three in ten runs of CI's browser lanes failed, on main as on pull requests, each time somewhere else. Four of the recurring places are races in the checks themselves, not in the app. Each was reproduced with a named delay before it was changed:

  • terminal-input-regression.ts:65, "input regression timed out" (5 times). The check waits for the "Waiting for terminal input…" banner to be hidden, then dispatches online and waits for a ping. The banner is drawn only on a connected pane, so before the socket opens it is hidden too, and wake() then has nothing to probe (probe() returns unless the socket is open and the snapshot has said ping). No later event sends one. The check now waits for .conn-live first.
    Reproduced by opening the page's first socket 3 s late: 2 of 2 failed with CI's message at that line; 3 of 3 pass with the fix under the same delay.
  • update-notice-regression.ts:74, "one tap asks for the install: 0 !== 1" (2 times). The line says "Starting the update…" as the button is pressed; the route that counts the request can run after that text is seen. The check now waits for the install request's response before it reads the count, for the first press and for Try again.
    Reproduced by counting the request 400 ms late: 2 of 2 failed with CI's message; 3 of 3 pass with the fix.
  • usage-regression.ts:157, "a hidden strip asks for nothing: 7 !== 6". Opening Settings mounts its own useUsage, which asks once; that request could be counted after the check had read its count and reloaded. The check now marks the reloaded page's requests with a header and asserts that none of those asked for usage.
    Reproduced by counting requests 150 ms late: 3 of 3 failed with CI's message; 3 of 3 pass with the fix. With plan limits switched back on before the reload the assertion still fails, as it must.
  • mobile-viewport-regression.ts:37, '' !== 'contain' (3 times). The transcript's first node is replaced as the conversation loads, 42 to 126 ms after the composer appears in three local runs. A locator that found the first node and read its style after the swap reads a node outside the document, whose computed style is empty. The check now finds the node and reads the style in one step in the page.
    The swap and the empty style of the replaced node were observed; the timing that lands the read between them was not reproduced.

No app code changes. sticky-modifiers and the split drop are #984's, chat-virtual is #985's.

Validation

  • macOS arm64, Bun 1.4.0, herdr 0.9.3. bun run typecheck: pass. scripts/conventions.test.ts, changelog-fold.test.ts, pages-gate.test.ts: 39 pass.
  • bun run check run --build bun scripts/ui-regression.ts: 65 PASS, exit 0.
  • bun scripts/ui-contexts-regression.ts: exit 0, all 24 checks.
  • The before/after runs above used a throwaway runner and delays that are not in this PR.
  • Not run locally: the integration lane and the other browser scripts.

terminal-input waited for a banner that is absent before the socket opens, then asked for a probe nothing could send. update-notice and usage counted a request by when the route saw it. mobile-viewport read the style of a transcript node that is replaced as the conversation loads. Each waits for, or reads, the thing it asserts.
@coderabbitai

coderabbitai Bot commented Oct 13, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: devswha/herdr-web-ui/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 911fff26-1349-4005-a9ab-8f87b81f2241
📥 Commits

Reviewing files that changed from the base of the PR and between 6f87b20 and 4a0e8f2.

📒 Files selected for processing: 5
  • changelog.d/browser-checks-wait-for-what-they-read.md
  • scripts/mobile-viewport-regression.ts
  • scripts/terminal-input-regression.ts
  • scripts/update-notice-regression.ts
  • scripts/usage-regression.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

gimso2x pushed a commit to gimso2x/herdr-web-ui that referenced this pull request Oct 14, 2026
## Change

CHANGELOG.md's `## [Unreleased]` lists are the repo's most frequent
merge conflict: every PR appends to the same Added/Changed/Fixed lists,
and a conflict resolver once pushed the markers (devswha#838). A PR now adds
one small file to `changelog.d/` instead, so two PRs never touch the
same lines.

- **Format** (`changelog.d/README.md`): a lowercase `name.md` holding
exactly the lines it would have in CHANGELOG.md: `###
Added|Changed|Removed|Fixed|Maintenance` headings, `- ` entries,
two-space continuations. No PR link or credit needed.
- **Fold** (`scripts/changelog-fold.ts`): the release PR runs `bun
scripts/changelog-fold.ts X.Y.Z [YYYY-MM-DD]` (the date defaults to the
local one). It turns the Unreleased notes into `## [X.Y.Z] - date` under
an empty `## [Unreleased]` (the shape of devswha#853), with the existing
Unreleased entries first, then the fragments sorted by file name,
sections in release order (any other heading the old notes used is kept
after them). It links each fragment's entries to the PR whose squash
merge added the file (from `git log --follow --diff-filter=A`, so a
later rename does not take the link) unless the entry already links that
PR, moves the compare links on, deletes the fragments, and prints what
still needs a link plus the reminder to add `by @login` credits. It
refuses rather than drops a line it cannot place.
- **Release gate**: `scripts/release-notes.ts`, which release.yml runs
before tagging, refuses while `changelog.d/` still holds a fragment (one
merged after the release PR folded them would otherwise ship without a
line in the notes), and says to move its entries into the version by
hand.
- **Transition**: the ~30 open PRs that still append to Unreleased need
no rework; their lines are folded together with the fragments.
CHANGELOG.md content is unchanged in this PR.
`scripts/release-notes.ts`, `server/release-notes.ts` and
`scripts/update-browser-qa.ts` read the same shape as before.
- **Guards** (`scripts/changelog-fold.test.ts`, unit suite): every
fragment parses (known section, at least one non-empty entry, no stray
text, lowercase `.md` name); CHANGELOG.md and the fragments hold no
conflict markers; today's Unreleased notes plus fragments fold for the
next release, so an old PR's malformed entry fails at its own PR rather
than at release. Failure messages name the file and line and point at
`changelog.d/README.md`. Not enforced yet: that a PR leaves CHANGELOG.md
alone. A ratchet or a `changes`-job check would fail the ~30 open PRs
that still append there (or turn main red when one of them merges on a
stale green run), so it is a planned follow-up once they have drained;
AGENTS.md says so.
- **Docs**: CONTRIBUTING.md (changelog bullet), AGENTS.md maintainer
workflow (one line, pointing at the test), docs/development.md Releasing
step 1, the PR template comment.

No CHANGELOG entry or fragment: not user-visible.

## Validation

- `bun run check fast`: workflow syntax, generated types, typecheck (the
fold script added to `tsconfig.tools.json`), build, unit (3032 pass, 0
fail, 8 skip).
- `bun test scripts/changelog-fold.test.ts
scripts/release-notes.test.ts`: 18 pass, including one where
`releaseNotes()` reads a folded changelog.
- `git log --follow --diff-filter=A` checked on a throwaway branch:
after `x (devswha#1000)` adds a fragment and `chore: rename (devswha#1001)` renames
it, it reports devswha#1000 (without `--follow`, devswha#1001).
- Dry run on this branch with a probe fragment: `bun
scripts/changelog-fold.ts 0.4.9 2026-10-13` changed CHANGELOG.md by only
the new heading, the probe line at the end of Fixed and the two compare
links (+5/-1), and deleted the fragment. Reverted afterwards.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@devswha
devswha merged commit d3d9d2e into main Oct 14, 2026
15 of 17 checks passed
@devswha
devswha deleted the test/deflake-browser-checks branch October 14, 2026 00:32
devswha added a commit that referenced this pull request Oct 14, 2026
…1009)

## Change

Since #1002 a browser script that fails is run once more and a pass on the second run is reported as flaky. Two things were still missing for whoever maintains the lanes.

**The report did not say what failed.** "FLAKY scripts/ui-regression.ts" names a script with 65 checks. The lane now reads the failed run's log for the first error line and the first stack frame in `scripts/`, and puts both in the console line, the warning annotation, the job summary and `summary.json`:

```
::warning title=Flaky browser script::scripts/ui-regression.ts: scripts/terminal-input-regression.ts:65: AssertionError: input regression timed out failed once and passed when run again. …
```

A script that fails twice gets an `::error title=Browser script failed::` annotation with the same, which it did not have. A log with no recognisable error is reported as before, by script alone.

**Nothing counted failures across runs.** `bun scripts/ci-flakes.ts [--hours 24] [--repo owner/name] [--json]` reads recent `ci.yml` runs through `gh`, every attempt of each, and prints each script and failing site with how often it failed and how often it passed only on a second run, most frequent first. It changes nothing. A failed job's log is read; a passed job's only when it carries the flaky warning, so 30 hours of this repository (242 runs, 516 browser jobs) take about 100 seconds instead of more than five minutes.

What it shows today, first rows of `--hours 30`:

| failed | flaky | script | site |
|---|---|---|---|
| 14 | 1 | ui-regression | terminal-input-regression.ts:65 (fixed in #1001) |
| 11 | 0 | ui-contexts-regression | split-view-regression.ts:81 |
| 10 | 0 | chat-virtual-browser-qa | chat-virtual-browser-qa.ts:169 |
| 9 | 0 | pane-paging-demo-regression | pane-paging-demo-regression.ts:279 |
| 8 | 0 | prompt-dock-demo-regression | (no script frame in the log) |

No change to which scripts run, to the second run, to exit codes or to what gates a merge.

## Validation

- macOS arm64, Bun 1.4.0. `bun run typecheck`: pass. `scripts/ci-flakes.test.ts` (new), `pages-gate.test.ts` (new cases for `failureSite`), `conventions.test.ts`, `changelog-fold.test.ts`: 0 fail. With `failureSite` made to return no site, 8 of those cases fail.
- The lane, run for real with `BROWSER_SCRIPTS=scripts/deep-link-demo-regression.ts GITHUB_ACTIONS=true bun scripts/ci-browser-parallel.ts` and a throwaway line that made that script fail:
  - an assertion on the first run only: exit 0, the FLAKY line, the `::warning`, the job summary and `summary.json` all carry `scripts/deep-link-demo-regression.ts:10` and `AssertionError: scratch forced failure`;
  - the assertion on both runs: exit 1, `::error title=Browser script failed::` with the same;
  - `process.exit(1)` with nothing printed, first run only: exit 0, reported flaky by script alone, no "null" or "undefined".
- `bun scripts/ci-flakes.ts --hours 30` against this repository: exit 0 in 103 s; lists `scripts/ui-regression.ts` as flaky for run 38490957637 and the pre-retry failures. `--json` parses. With `GH_TOKEN` set to a bad token it prints `gh: Bad credentials (HTTP 401)` and exits 1.
- Not run locally: the integration lane and the browser lanes as a whole.
devswha added a commit that referenced this pull request Oct 14, 2026
…not after input-ready

#1001 made the check wait for the connection, and it still needed a second run about one time in three. A diagnostic CI run shows why: input-ready arrived at +260 ms, the check dispatched `online` at +340 ms, and the snapshot that names the bridge's features (ping among them) arrived at +348 ms, so the wake probed nothing. Reproduced by holding only the snapshot frame back.
devswha added a commit that referenced this pull request Oct 14, 2026
…est survives a pull limit (#1016)

## Change

Two CI failures that are not a change's fault, each reproduced before it was changed.

**`prompt-dock-demo-regression.ts`: "waiting for locator('.prompt-card') to be detached", 30 s, 8 times in the last 30 hours.** After the second asking of a question is answered, the check waited for any `.prompt-card` to go. The demo has an approval of its own, and the page reads it as soon as the asked question is answered: when the 2 s prompt poll lands before the answer returns, that card takes the place at once and no moment without a card is ever seen. The call log shows it: the card busy, then 60-odd polls of a card that is not. The check now waits for the card of the question it answered, as its neighbours already do.
Reproduced by holding the answer back 2.5 s (past one poll): exit 1 with CI's timeout and call log. With the fix it passes under the same delay and without it.

**The `ssh` job: `docker run failed … toomanyrequests`.** `scripts/ssh-password-docker.ts` starts its disposable PC from `public.ecr.aws/docker/library/debian:bookworm-slim`, and the registry refuses a shared runner's address once it is over its anonymous pull limit. That failed the job on two unrelated PRs yesterday. The script now pulls the image first: when a registry answers with a rate limit it asks `mirror.gcr.io` and then Docker Hub, and all three once more after 20 s, before it fails. Any other refusal still fails at once.
Shown with a `docker` stand-in on PATH: two limited pulls then a good one go on to `docker run` with the third image; a pull that says "manifest unknown" fails at once; pulls limited every time fail after six tries and 20 s with docker's message.

Not in this PR: `terminal-input-regression.ts` still needs a second run about one time in three on `main` after #1001 (2 of 6 in a 12-job run of the browser lanes), at the same wait. I did not reproduce it locally and am reading it from a diagnostic CI run; `terminal-copy-regression.ts:274` was not reproduced either (two attempts) and is left as it is.

## Validation

- macOS arm64, Bun 1.4.0. `bun run typecheck`: pass. `scripts/conventions.test.ts` (fixed-sleep counts unchanged), `changelog-fold.test.ts`: pass.
- `bun scripts/prompt-dock-demo-regression.ts`: exit 0, 24 PASS, with the answer held back 2.5 s and without; exit 1 with the timeout before the change under the same delay.
- `scripts/ssh-password-docker.ts` with the `docker` stand-in: the three cases above. The real script against Docker was not run here (no Docker on this Mac); CI's `ssh` job runs it.
- Not run locally: the integration lane and the other browser scripts.
devswha added a commit that referenced this pull request Oct 14, 2026
…not after input-ready (#1019)

## Change

`terminal-input-regression.ts` fails with "input regression timed out" more than any other browser check: 14 failures and 1 second-run pass in the last 30 hours of CI. #1001 made it wait for the connection before it asks for a liveness probe, and that was not the cause: on `main` with #1001 the check still needed a second run in 2 of 6 runs of the lane.

A CI run with diagnostics printed on the timeout shows the order of frames on the runner:

```
+259 <- role-ack interact
+260 <- pty-data, input-ready, pane-geometry
+340 dispatched online
+348 <- snapshot features=…|ping
+10358 -> ping            (the idle path, after the check had given up)
```

The snapshot is what tells the client the bridge answers pings. A wake before it probes nothing (`probe()` returns while `pingKnown` is false), and nothing asks again. On a development machine the snapshot arrives first (+114 ms against +213 ms here), so the check never failed locally and #1001's reproduction found another way to time out, not this one. The page was visible and connected, and a second `online` after the timeout did send the ping.

The check now notes when the snapshot was forwarded to the page and waits for that before it dispatches `online`. No app code changes: a wake in the first few milliseconds of a connection has nothing stale to find.

This commit was meant for #1016 and was pushed seconds after it merged.

## Validation

- Reproduced by holding only the snapshot frame back 1.5 s: 2 of 2 fail with CI's message at that wait on `main`'s code. With the change: 3 of 3 pass under the same hold, 3 of 3 without it.
- `bun run check run --build bun scripts/ui-regression.ts`: 65 PASS, exit 0 (macOS arm64, Bun 1.4.0). `bun run typecheck`, `scripts/conventions.test.ts`, `changelog-fold.test.ts`: pass.
- On CI: `scripts/ui-regression.ts` run six times on this change, one script per job (run 38497470872, a throwaway branch): 6 of 6 passed on the first run. On `main` before it the check needed a second run in 2 of 6 jobs of the same shape (run 38495833732) and in 2 of 6 jobs of the whole lane, which runs two scripts side by side (run 38494284688). Six clean runs fit the cause above; they do not prove the check can never time out again.
- Not run locally: the integration lane and the other browser scripts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant